terraform/aws: remove option to use an existing vpc in aws #654

staebler · 2018-11-11T18:43:52Z

For the limited scope of the installer, we do not want the user to
have the ability to share the VPC between clusters. A shared VPC
could potentially be deleted when destroying one of the clusters,
leaving the rest of the clusters using the shared VPC in an unusable
state.

Fixes https://jira.coreos.com/browse/CORS-873

wking · 2018-11-15T21:06:32Z

Needs a rebase?

data/data/aws/vpc/common.tf

pkg/tfvars/tfvars.go

pkg/types/aws/platform.go

cuppett · 2018-11-16T16:24:00Z

I strongly believe we need to keep both the CIDR and the existing VPC support. In fact, we may want to enhance it a bit to use all or a subset of AZs.

Can we instead fix/protect the destroy logic?

crawford · 2018-11-29T00:29:48Z

/retest
/lgtm

We are going to punt on this functionality until after 4.0.

openshift-ci-robot · 2018-11-30T18:56:59Z

@staebler: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	4ff2ec8aba0834d709d325cce27191270fe59f33	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2018-12-05T18:45:53Z

Needs a rebase :).

data/data/aws/master/main.tf

pkg/tfvars/aws/aws.go

abhinavdahiya · 2018-12-13T23:06:12Z

@wking can you take another look otherwise this is /lgtm

/approve

wking · 2018-12-13T23:15:32Z

data/data/aws/bootstrap/main.tf

@@ -109,7 +109,7 @@ resource "aws_instance" "bootstrap" {
  subnet_id                   = "${var.subnet_id}"
  user_data                   = "${data.ignition_config.redirect.rendered}"
  vpc_security_group_ids      = ["${var.vpc_security_group_ids}"]
-  associate_public_ip_address = "${var.associate_public_ip_address}"
+  associate_public_ip_address = "true"


I think we want to drop the quotes, see here.

@wking I thought that I had fixed that already. I'll get that done.

Oh, that's a different one. I fixed the one in master.

wking · 2018-12-13T23:20:13Z

data/data/aws/main.tf

@@ -1,7 +1,7 @@
 locals {
  private_endpoints = "${var.aws_endpoints == "public" ? false : true}"
  public_endpoints  = "${var.aws_endpoints == "private" ? false : true}"
-  private_zone_id   = "${var.aws_external_private_zone != "" ? var.aws_external_private_zone : join("", aws_route53_zone.int.*.zone_id)}"
+  private_zone_id   = "${join("", aws_route53_zone.int.*.zone_id)}"


Do we know what this join is about? I'd expect "${aws_route53_zone.int.zone_id}".

@wking The join is used to convert the list to a single value, knowing that the list will only have a single element. With the changes in this PR, the join is no longer necessary aws_route53_zone is no longer a collection of resources: it is a single resource.

And to be clear, the code in the PR is what you are expecting "${aws_route53_zone.int.zone_id}". Your comment is using an outdated version.

Ah, I had been going through commit by commit. Maybe you can squash the join removal further back. Or just leave it, since it gets fixed by the end of the PR.

I try to make the end result of each commit a working product. After that commit, the join is still needed since at that point aws_route53_zone is still a collection of resources.

For the limited scope of the installer, we do not want the user to have the ability to share the VPC between clusters. A shared VPC could potentially be deleted when destroying one of the clusters, leaving the rest of the clusters using the shared VPC in an unusable state. Fixes https://jira.coreos.com/browse/CORS-873

With the removal of the option to use an external VPC, the following variables are not used and are removed. aws_external_master_subnet_ids aws_external_private_zone aws_external_worker_subnet_ids https://jira.coreos.com/browse/CORS-873

aws_endpoints aws_installer_role aws_master_custom_subnets aws_master_extra_sg_ids aws_worker_custom_subnets https://jira.coreos.com/browse/CORS-873

wking · 2018-12-14T22:20:44Z

/lgtm

openshift-ci-robot · 2018-12-14T22:20:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, staebler, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,crawford,staebler,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Catching up with openshift/installer@6f55e673 (terraform/aws: remove option to use an existing vpc in aws, 2018-11-11, openshift/installer#654).

Centralize extra-tag inclusion on aws/main.tf. This reduces the number of places we need to think about what tags should be ;). Also keep kubernetes.io/cluster/{name} localized in the aws module. See 8a37f72 (modules/aws/bootstrap: Pull AWS bootstrap setup into a module, 2018-09-05, openshift#217) for why we need to keep it on the bootstrap instance. But the bootstrap resources will be removed after the bootstrap-complete event comes through, and we don't want Kubernetes controllers trying to pick them up. This commit updates the internal Route 53 zone from KubernetesCluster to kubernetes.io/cluster/{name}: owned, catching it up to kubernetes/kubernetes@0b5ae539 (AWS: Support shared tag, 2017-02-18, kubernetes/kubernetes#41695). That tag originally landed on the zone back in 75fb49a (platforms/aws: apply tags to internal route53 zone, 2017-05-02, coreos/tectonic-installer#465). Only the master instances need the clusterid tag, as described in 6c7a5f0 (Tag master machines for adoption by machine controller, 2018-10-17, openshift#479). A number of VPC resources have moved from "shared" to "owned". The shared values are from 45dfc2b (modules/aws,azure: use the new tag format for k8s 1.6, 2017-05-04, coreos/tectonic-installer#469). The commit message doesn't have much to say for motivation, but Brad Ison said [1]: I'm not really sure if anything in Kubernetes actually uses the owned vs. shared values at the moment, but in any case, it might make more sense to mark subnets as shared. That was actually one of the main use cases for moving to this style of tagging -- being able to share subnets between clusters. But we aren't sharing these resources; see 6f55e67 (terraform/aws: remove option to use an existing vpc in aws, 2018-11-11, openshift#654). [1]: coreos/tectonic-installer#469 (comment)

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2018

openshift-ci-robot requested review from rajatchopra and wking November 11, 2018 18:43

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 11, 2018

abhinavdahiya mentioned this pull request Nov 15, 2018

data/aws/route53/variables: Drop unused extra_tags #685

Merged

staebler changed the title ~~[WIP] terraform/aws: remove option to use an existing vpc in aws~~ terraform/aws: remove option to use an existing vpc in aws Nov 16, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2018

wking reviewed Nov 16, 2018

View reviewed changes

data/data/aws/vpc/common.tf Outdated Show resolved Hide resolved

wking reviewed Nov 16, 2018

View reviewed changes

pkg/tfvars/tfvars.go Show resolved Hide resolved

wking reviewed Nov 16, 2018

View reviewed changes

pkg/types/aws/platform.go Show resolved Hide resolved

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2018

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 16, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2018

openshift-ci-robot assigned crawford Nov 29, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 29, 2018

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 3, 2018

wking reviewed Dec 5, 2018

View reviewed changes

data/data/aws/master/main.tf Outdated Show resolved Hide resolved

wking reviewed Dec 5, 2018

View reviewed changes

pkg/tfvars/aws/aws.go Outdated Show resolved Hide resolved

wking reviewed Dec 5, 2018

View reviewed changes

pkg/tfvars/aws/aws.go Show resolved Hide resolved

wking reviewed Dec 13, 2018

View reviewed changes

staebler added 3 commits December 13, 2018 19:46

terraform: remove some more unused aws variables

0471938

aws_endpoints aws_installer_role aws_master_custom_subnets aws_master_extra_sg_ids aws_worker_custom_subnets https://jira.coreos.com/browse/CORS-873

openshift-ci-robot assigned wking Dec 14, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2018

openshift-merge-robot merged commit 3b9d99b into openshift:master Dec 14, 2018

staebler mentioned this pull request Dec 15, 2018

asset/installconfig: improve validation of user inputs #711

Merged

wking added a commit to wking/hive that referenced this pull request Dec 19, 2018

pkg/apis/hive/v1alpha1: Drop VPCID

99124d7

Catching up with openshift/installer@6f55e673 (terraform/aws: remove option to use an existing vpc in aws, 2018-11-11, openshift/installer#654).

wking mentioned this pull request Feb 4, 2019

pkg/tfvars/aws: Drop custom subnets #698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

terraform/aws: remove option to use an existing vpc in aws #654

terraform/aws: remove option to use an existing vpc in aws #654

staebler commented Nov 11, 2018

wking commented Nov 15, 2018

cuppett commented Nov 16, 2018

crawford commented Nov 29, 2018

openshift-ci-robot commented Nov 30, 2018 •

edited

Loading

wking commented Dec 5, 2018

abhinavdahiya commented Dec 13, 2018

wking Dec 13, 2018

staebler Dec 14, 2018

staebler Dec 14, 2018

wking Dec 13, 2018

staebler Dec 14, 2018

staebler Dec 14, 2018

wking Dec 14, 2018

staebler Dec 14, 2018

wking commented Dec 14, 2018

openshift-ci-robot commented Dec 14, 2018

terraform/aws: remove option to use an existing vpc in aws #654

terraform/aws: remove option to use an existing vpc in aws #654

Conversation

staebler commented Nov 11, 2018

wking commented Nov 15, 2018

cuppett commented Nov 16, 2018

crawford commented Nov 29, 2018

openshift-ci-robot commented Nov 30, 2018 • edited Loading

wking commented Dec 5, 2018

abhinavdahiya commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Dec 14, 2018

openshift-ci-robot commented Dec 14, 2018

openshift-ci-robot commented Nov 30, 2018 •

edited

Loading